16079 Many fixes to Approval Type component #257
Conversation
- fixed whitespace - simplified v-radio template - fixed validations - fixed change/input/error handling - added validate prop - updated court order number rules text - fixed initial state (when mounted) - fixed validation - fixed lint issues
severinbeauvais
requested review from
JazzarKarim,
ketaki-deodhar and
ArwenQin
|
Hello. Please review these changes while I work on the unit tests. I am aware that some of these changes will break existing users of this component. For this reason, I will update the minor version number of this component (ie, from 1.0.61 to 1.1.0). (I can't use 2.x.x because it's already used for Vue3 code.) |
| <div | ||
| id="approval-type" | ||
| :class="{ 'invalid-section': invalidSection }" | ||
| > |
| > | ||
| <v-radio-group | ||
| v-model="approvalTypeSelected" |
There was a problem hiding this comment.
It seemed redundant to use both a v-model property and handle the change event so I went with :value and @change to keep things consistent with the other components in here.
| label="Court Order Number" | ||
| :rules="courtOrderNumRules" | ||
| :value="courtOrderNumber" | ||
| :rules="validate ? courtOrderNumRules : []" |
There was a problem hiding this comment.
The rules are conditional on the validate prop (but you have to wait a tick for validation to happen).
| hide-details="auto" | ||
| filled | ||
| @input="courtOrderNumberChanged" | ||
| @update:error="emitValidationError($event)" |
There was a problem hiding this comment.
Redundant. Now handled more elegantly below.
|
|
||
| /** Show only the court order option; remove via registrar option. */ | ||
| @Prop({ default: false }) readonly isCourtOrderOnly!: boolean |
There was a problem hiding this comment.
I replaced this with approvedByCourtOrder (similar to approvedByRegistrar), which keeps the flag isCourtOrderRadio, below, functionally separate -- they do separate things instead of being dependent on each other.
| } | ||
| if (!this.isCourtOrderRadio) { | ||
| this.approvalTypeSelected = ApprovalTypes.VIA_COURT_ORDER |
There was a problem hiding this comment.
There is no more need for "default" state handling. When we come in to this component, we should have either approvedByRegistrar or approvedByCourtOrder set.
If both are set (in error) then only the first one will have an effect.
If neither are set (in error) then no radio buttons are pre-selected, but the component should still function correctly once the user selects one.
| if (this.approvalTypeSelected === ApprovalTypes.VIA_COURT_ORDER) { | ||
| let status = this.$refs.courtNumRef.validate() | ||
| this.$emit('valid', status) |
There was a problem hiding this comment.
We shouldn't use this all over the code. We should have a single place where a property is emitted. This is now THIS method (as per @Emit decorator).
|
See bcgov/entity#16079 for more screenshots. |
severinbeauvais
changed the title
severinbeauvais
changed the title
- updated/fixed unit tests
Issue #: bcgov/entity#16079
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the business-filings-ui license (Apache 2.0).